Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve compatibility with Luxafor Bluetooth Pro devices #9

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tups
Copy link

@tups tups commented Jul 22, 2024

This Pull Request addresses a compatibility issue with Luxafor Bluetooth Pro devices, specifically resolving a problem where the external indicator wasn't changing color properly.

Main changes:

  • Modified the SetColor method to explicitly pass null as the fadeInTime value.
  • This change affects both the shutdown process and the color-changing process.

Technical details:

  • By passing null for fadeInTime, we force the use of the "JumpTo" command instead of "FadeTo".
  • This ensures that color changes are properly transmitted to all connected devices, including Bluetooth ones.

Testing:

  • Successfully tested on Luxafor Bluetooth Pro devices.
  • Verified that the external indicator changes color as expected.

Impact on the project:

  • Improves compatibility with a wider range of Luxafor devices.
  • Does not affect the functionality of existing non-Bluetooth devices.

Additional notes:

  • It would be beneficial to update the project documentation to mention this compatibility improvement.
  • Further testing on different Luxafor device models would be valuable to ensure full compatibility.

Please review these changes and feel free to provide any feedback or suggestions for improvement.

In connection with the issue: #8

@jschlackman
Copy link
Owner

Looks great, good job identifying the underlying incompatibility with the fadeInTime.

Can you confirm if the Wave and Blink options work correctly on the Bluetooth Pro without having to modify the fadeInTime for those commands?

@jschlackman jschlackman self-assigned this Jul 22, 2024
@jschlackman jschlackman linked an issue Jul 22, 2024 that may be closed by this pull request
The `long` option on the Wave method does not work on Luxafor Bluetooth Pro, so I propose the OverlappingLong alternative which works.
For the Blink method with a single repetition does not work on Luxafor Bluetooth Pro. With 2 repetitions it works
@tups
Copy link
Author

tups commented Jul 23, 2024

I have just done several tests on the Luxafor Bluetooth Pro.

Here are the results:

Wave Wave Type LuxaforSharp Constant Luxafor Bluetooth Pro Test
4 1 WaveType.Short
4 2 WaveType.Long
4 3 WaveType.OverlappingShort
4 4 WaveType.OverlappingLong
4 5 ?

I also notice that the device.Wave method, if the repeatCount parameter is zero, the Wave does not work.

For the Blink function, it works directly but not from the LuxaforOnAir application, and I make almost the same observation, if changes the repeatCount parameter to 2 instead of 1 on the device.Blink method, then it works


This additional commit addresses compatibility issues with Luxafor Bluetooth Pro devices, specifically improving the functionality of the Wave and Blink methods.

Main changes:

  1. Modified the Wave method to use WaveType.OverlappingLong instead of WaveType.Long.
  2. Changed the repeat count for both Wave and Blink methods from 0/1 to 2.

Technical details:

  • Wave method: WaveType.Long doesn't work on Luxafor Bluetooth Pro, but WaveType.OverlappingLong does.
  • Blink and Wave methods: A repeat count of 0 or 1 doesn't work, but 2 repetitions function correctly.

Testing results for Wave types on Luxafor Bluetooth Pro:

Wave Type Result
Short
Long
OverlappingShort
OverlappingLong
Unknown (Wave Type 5)

Please review these changes and provide any feedback or suggestions for improvement. If you have any questions about the test results or the implemented changes, feel free to ask.

@jschlackman
Copy link
Owner

Thanks, I reviewed these changes with the standard Luxafor light today and we're close to a fix.

The repeatCount of 2 for the Blink behavior works fine, and I will adjust the internal timer to compensate for the longer blink effect (I've been testing with 6s instead of 3s).

The Wave change has a couple issues, one of which is easy to address and the other more time-consuming.

The overlapping wave types combine the requested color (e.g. red) with the previous color, so we get a wave between red and green, which is very different from waving only red (which is what the standard short/long wave does). We can work around this by inserting the following line immediately before the Wave function is called on line 118. This causes the red wave to overlap with an almost-off solid red light, which comes very close to the look of the standard wave.

device.SetColor(LedTarget.All, new LuxaforSharp.Color(1, 0, 0), null);

Changing the repeatCount to 2 is a more complicated problem. The blink and wave behaviors in LuxaforOnAir are implemented slightly differently: as mentioned above, Blink uses a timer within the app to repeat the blink periodically, but Wave relies on the constant repeat that a repeatCount of 0 is supposed to perform. By changing the repeatCount to 2, it waves twice - and then stops waving and remains solid.

I'll likely come back to this tomorrow but at this time I am minded to accept the PR as-is and figure out an adjustment to compensate for the changed Wave behavior (probably using the internal timer currently in use for Blink).

As an aside, I reviewed the code for the LuxaforSharp library I use to interface with the lights to see if there was an obvious incompatibility that could be addressed, but I can't see any reason that these commands work differently on the Bluetooth Pro. It's disappointing that Luxafor have not implemented the commands for the newer model in a way that is consistent with the earlier models.

@tups
Copy link
Author

tups commented Jul 25, 2024

I confirm that LuxaforSharp does not have any bugs, it is the Luxafor Bluetooth Pro that has the bug.
Because to do my tests, I used the official dev tool: https://luxafor.com/wp-content/uploads/2023/08/Luxafor-for-developers.zip

Inside, there is documentation with the expected behaviors and the data to send to the HID, and I tested the raw calls directly from their dev tool to do the tests and my comparison table above.

If I have some time, I will complete my table above with all the features (repetition tests, by type, etc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Choose the device (e.g. Luxafor Bluetooth Pro)
2 participants